feat: unconditional control-plane self-registration with no-auth support HYPERSHELL-297 - #284

Verdict

The earlier spec/security contradictions (client-side vs server-side watch filtering, external-gRPC auth) and the em-dash CI gate stay resolved, which is real progress. But the registration model still keys records on name alone while the specs promise identity-scoped watch authorization, and the unique-name migration still tightens a constraint with no dedup/backfill for pre-existing data, so the verdict stays REQUEST_CHANGES.

Summary

At this head the code path is clean (no panic, errors wrapped, no secrets logged, name validated as a DNS label, make check / forbidden-terms passes locally). The two Major concerns from the prior review are unchanged in the code, so this is still REQUEST_CHANGES; all findings already have Amber inline threads, which I link rather than duplicate.

Findings

[Major] Watch spec promises identity-scoped authorization the name-only registration model cannot provide. global-architecture.spec.md:548-551 says the API server scopes watch streams to the caller's cluster_id and "authorizes it against the caller's authenticated identity," and control-plane.spec.md:214 says the filter is server-side. But service.go:172-179 looks up the record by name and, on the existing-record branch, only updates LastSeenAt and returns the stored cluster_id; it never compares the caller's oidc_subject to existing.OIDCSubject, and oidc_subject is documented as audit-only (managed-cluster-registration.spec.md "Data Model Impact" / "Idempotent Registration"). With no durable identity->cluster_id binding, the server has nothing to authorize the watch's cluster_id against, and any managed-cluster-registrar holder can register another control plane's name, receive that cluster_id, and open its watch. This is a maintainer design decision: either bind the record to identity, or drop the identity-scoped-authorization claim. Existing thread: #284 (comment). Confidence: High on the inconsistency.

[Major] Unique-name migration tightens uniqueness with no dedup/backfill for pre-existing data. migration.go:47-51 (and the rewritten :21-25) creates CREATE UNIQUE INDEX ... uix_managed_clusters_name ON managed_clusters (name) WHERE deleted_at IS NULL. On main the only uniqueness was (oidc_subject, name), so two control planes with different subjects could already hold the same name. If such duplicate live rows exist in a database that ran the main version of this feature, the index creation errors, the migration aborts, and the API server fails to start. Add a dedup step before the index, or document why duplicates cannot exist in any running environment. Existing thread: #284 (comment). Confidence: Medium (depends on whether the main version ran against real data).

[Minor] An already-released migration is being edited in place. migrationAddRegistrationFields (ID 2026091000000001, migration.go:10-38) already shipped on main; rewriting its body to create the name-only index (:21-25) is dead code in any DB where it already ran (gormigrate skips by ID) - only the new migrationRegistrationKeyToNameOnly (ID 2026092300000001, :40-64) takes effect there - and it makes rollback inconsistent (the old migration's rollback now drops the name index while the new migration's rollback recreates the oidc_subject index). Revert this migration to its shipped definition and rely solely on the new migration. Existing thread: #284 (comment). Confidence: High.

[Minor] DefaultManagedClusterName = "local" is a silent collision footgun. config.go:31,103 defaults HYPERSHELL_MANAGED_CLUSTER_NAME to the literal local when unset. Combined with name-as-sole-key, any overlay that forgets to set this variable registers as local and shares one cluster_id with every other such control plane. The Kind (local-kind) and OpenShift (local-openshift) overlays set explicit names, but the code default is still local and there is no fail-fast, so ROKS / remote-managed-cluster overlays that omit the variable still collide silently. This also answers the open inline question on the Kind overlay (#284 (comment)): yes, every non-dev overlay must set a unique name; OpenShift now does, but the shared code default remains a trap. Consider failing fast when unset in an auth-enabled deployment. Existing thread: #284 (comment). Confidence: High.

Note (not a blocker): factory_test.go changing the test cluster Name from the shared literal "test-name" to the unique id is the expected fallout of the new global name-uniqueness constraint; it is fine, but it confirms the shared precondition was tightened.

Cross-PR coordination

A competing full implementation of this same feature is open in #362 ("mandatory control-plane cluster identity, hub gRPC TLS, cluster-scoped watch streams"). It edits the same functions and files this PR does (managedClusters/service.go, dao.go, migration.go, mock_dao.go, control-plane/internal/config/config.go, registration/client.go, and the same platform specs) but chooses the opposite registration/identity model. This PR renames the lookup to FindByName, makes name the sole upsert key, defaults it to local, treats a name collision as idempotent adoption of the existing record, and supports a no-auth path with an empty oidc_subject. #362 keeps oidc_subject as the owning identity, makes name+OIDC mandatory (removing HYPERSHELL_CLUSTER_ID), returns 409 on a name held by a different/empty subject, forbids silent adoption, and adds identity-based lookups precisely to authorize watch streams. These are mutually exclusive contracts for Register and for the DAO interface, and #362 is the concrete resolution of the identity-binding gap this review flags as a Major finding. Maintainers must decide which registration/identity model is canonical and in what order the two land; they cannot both merge as written.

A second decision is shared with #182 ("enforce management API JWT audience"). This PR's HYPERSHELL-333 section (global-architecture.spec.md:481-484) requires the externally exposed gRPC watch to enforce the caller's OIDC token and scope streams to its cluster_id, and states the in-cluster JWT bypass "cannot hold once the endpoint is internet-facing." #182 hardens management-API JWT audience validation while keeping the opposite contract in oidc-integration.spec.md (gRPC watch methods as a trusted in-cluster path, with a gRPC auth-bypass list). Maintainers must decide whether watch methods bypass JWT and reconcile the two specs before either lands.

A third coordination point exists with #185 ("periodic world synchronization"). It defines a "complete inventory" / paginated API-inventory reconcile pass that lists resources (including Gateway) and drives convergence and orphan cleanup from it, without cluster_id scoping on the inventory list. This PR establishes that each control plane owns only its cluster_id's gateways and must never reconcile or tear down another cluster's gateways, scoping both WatchGateways and the seed listing server-side by cluster_id. The two must agree on whether the world-sync inventory list is itself scoped by cluster_id; an unscoped "complete inventory" in the pull model could converge or clean up gateways outside a control plane's ownership. The owners should decide the canonical inventory-scoping contract before either lands.

Previous concerns

• [Critical] CI blocker: em dashes + stale whitelist (#284 (comment)) - addressed. scripts/check_forbidden_terms.py exits 0 at this head (reproduced locally).
• [Major] Client-side vs server-side cluster_id filtering (#284 (comment), #284 (comment)) - addressed. control-plane.spec.md:214 and global-architecture.spec.md:543-552 now describe a single server-side filter; the client-side "ignore the event" language is gone. (The residual identity-binding gap is tracked as a Major finding above.)
• [Major] External gRPC watch lacks authN/authZ (#284 (comment)) - addressed. global-architecture.spec.md:481-484 now requires the external gRPC watch to enforce the OIDC token and scope streams to the caller's cluster_id.
• [Minor] Idempotency key stated name-only (#284 (comment)) - addressed as an intentional design change. managed-cluster-registration.spec.md documents name as the sole upsert key; spec and code are internally consistent. The security trade-off is raised as a Major finding above for a maintainer decision.
• [Major] Name-only upsert has no identity check on takeover (#284 (comment)) - still present (service.go:172-179 updates only LastSeenAt and returns the stored cluster_id, no oidc_subject comparison).
• [Major] Unique-name index has no dedup/backfill (#284 (comment)) - still present (migration.go:47-51).
• [Minor] Editing an already-released migration (#284 (comment)) - still present (migration.go:10-38).
• [Minor] Defaulting the registration name to local (#284 (comment)) - still present (config.go:31; the OpenShift overlay now sets local-openshift, but the code default and lack of fail-fast remain).

Findings Summary (ordered by severity, highest first)

1. [Major] Watch spec promises identity-scoped authorization the name-only registration model cannot provide (takeover via name) - Security / Spec Consistency (global-architecture.spec.md:548-551; managed-cluster-registration.spec.md; service.go:172-179)
2. [Major] Unique-name migration tightens uniqueness with no dedup/backfill for pre-existing data - Migration Safety (migration.go:21-25,47-51)
3. [Minor] Already-released migration edited in place (immutability) - Convention (migration.go:10-38)
4. [Minor] DefaultManagedClusterName="local" collision footgun for overlays that omit the variable - Config / Robustness (config.go:31,103)

Convention Checklist